Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Pass the correct kwargs to the cleanupsyncs management command. #11854

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Feb 10, 2024

Summary

  • The morango cleanupsyncs command takes push and pull kwargs to specify push and pull cleanups, but our task wrapper is currently passing is_push and is_pull
  • Rectifies this by correcting the kwargs after the validation has happened
  • Updates the relevant test

References

No extant issue - noticed while doing some testing on Django 3.2 upgrading, where Django now complains if you pass non-existent kwargs to a management command!

Reviewer guidance

I ran the tests, with the tweak they passed.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles requested a review from bjester February 10, 2024 23:23
@rtibbles rtibbles added the TODO: needs review Waiting for review label Feb 10, 2024
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Feb 10, 2024
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to pass in the correct arguments to begin with, here https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/kolibri_plugin.py#L54-L55

@rtibbles
Copy link
Member Author

Sure - I was going for the fewest changes, but if it's fine to change the function signature, then happy to do that.

@rtibbles rtibbles force-pushed the cleanupsyncs_push_me_pull_you branch from 76ceb9f to 7dd81e5 Compare February 12, 2024 16:23
@rtibbles
Copy link
Member Author

Updated!

@bjester bjester self-assigned this Feb 12, 2024
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prathamesh and I made sure the test coverage properly covered this, but obv improperly asserted these. So looks good to me!

@bjester bjester merged commit 5c30fde into learningequality:release-v0.16.x Feb 12, 2024
34 checks passed
@rtibbles rtibbles deleted the cleanupsyncs_push_me_pull_you branch February 12, 2024 21:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants